Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ds #508

Open
wants to merge 67 commits into
base: master
Choose a base branch
from
Open

ds #508

wants to merge 67 commits into from

Conversation

jdifek
Copy link

@jdifek jdifek commented Sep 26, 2024

@TarasHoliuk
Copy link

  1. Fix lint errors:

image

  1. It seems like you're trying to hardcode card width (in percent) on the home page here exactly as it is in design:
    image

On design, this card width is an example of how it might look on this screen size. But you can try to adapt it - set 100% and, maybe set some max-width and min-width to handle edge cases and make sure it always looks good.

Copy link

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you partially implemented the mobile version. Please, re-request review when the entire task will be done

If you have questions or need help ask in the chat 🙂

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

  • Screenshot 2024-10-21 at 16 20 59
  • Check styles here
  • Screenshot 2024-10-21 at 16 20 35
  • Screenshot 2024-10-21 at 16 19 55
  • Consider removing the label if there are no products here
  • Screenshot 2024-10-21 at 16 20 20
  • Check the number here
  • Screenshot 2024-10-21 at 16 19 19
  • No need to show pagination if all is selected
  • Screenshot 2024-10-21 at 16 18 54
  • Check an alignment here
  • Screenshot 2024-10-21 at 16 17 54
  • Screenshot 2024-10-21 at 16 18 02
  • Check the layout for large desktops
  • Screenshot 2024-10-21 at 16 17 12
  • Consider adding links to your GH

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to increase this number in such cases

Screen.Recording.2024-10-21.at.22.35.32.mov
  1. why do we need this style
image
  1. add some gap between them, it's really hard to tap on some small link with a finger
image

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!
To improve:

  1. The text in buttons shouldn't wrap
image
  1. These buttons must be are on the same vertical line
image
  1. All these elements must be links and add hover effects here, also, open these additional links in the new tab
image
  1. Add transition when this menu appears on the page
image
  1. The slider isn't working on the mobile version, I can't change image
image

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GJ!
To improve:

  1. Fix the height for the slider buttons
image
  1. Add transition for hover effects everywhere
image
  1. This comment still not fixed from the previous review
image
  1. Open these additional links in the new tab
image
  1. The height for the slider images must be are same everywhere
image
  1. This comment still not fixed. The picture should change if we scroll through the picture itself
image

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to fix previous comments

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

items on page all don't work correctly
image
you should show all cards

  1. it's better to display number near these icons on mobile too
image

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

  • Consider fixing linting errors
  • Screenshot 2024-11-01 at 17 27 45
  • The product quantity change doesn't work correctly in the cart
  • Screenshot 2024-11-01 at 17 24 46
  • Also, the desktop has a horizontal scroll

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants